Skip to content

Conversation

@ldematte
Copy link
Contributor

We are already defaulting to entitlements for JDK24+ in #119885; this PR removes some dead code we had left in bootstrap.

@ldematte ldematte added >non-issue :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged test-entitlements v8.18.1 v8.19.0 v9.0.1 v9.1.0 labels Feb 12, 2025
@ldematte ldematte requested a review from a team as a code owner February 12, 2025 10:36
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 12, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

);
} else if (RuntimeVersionFeature.isSecurityManagerAvailable()) {
} else {
assert RuntimeVersionFeature.isSecurityManagerAvailable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An assert won't run in production/snapshot code, so if eg someone testing a snapshot or RC tries running with JDK24, they will get a nasty error from trying to use SM below. Could we make this more explicit with a helpful error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That like will always be true unless we make a change to code - the change in your PR ensures it. I added the assert to protect against ourselves (if we change something, and open the possibility of having neither, things in CI would start to fail).
I can make the change if you want of course, but no one should ever see that error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm still missing something: wouldn't a user that say uses a custom JVM of 24 on a snapshot of 9.0 beta hit this?

Copy link
Contributor Author

@ldematte ldematte Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already guarantee we use entitlements when we have a JVM 24:

final boolean useEntitlements = RuntimeVersionFeature.isSecurityManagerAvailable() == false || entitlementsExplicitlyEnabled;

(initPhase1)

So with 24 we always hit the entitlements branch.
The assert is here in case we change that logic and we don't realize it, leaving us uncovered again, but it is not strictly needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you! That's what I was missing.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldematte ldematte merged commit 556281a into elastic:main Feb 13, 2025
22 checks passed
@ldematte ldematte deleted the entitlements/remove-unused-bootstrap-else branch February 13, 2025 21:08
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Feb 13, 2025
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Feb 13, 2025
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants